Fail fast on terminal engine-down errors (402/balance exhausted/auth-expired) - #33

Review verdict: we want this capability, and we want it once. @mlava's #35/#36 solve the sibling problem — spawn-level failures (missing binary, exit 126/127) — with a second run-scoped circuit breaker living in the same two functions as yours. Merging both as written gives the codebase two parallel engine-health mechanisms with contradictory logging conventions, so instead of us picking a winner:

@jeffhamons @mlava — would you two put your heads together and design the canonical mechanism? One PR (co-authored, or stacked with clear layering — your call; authorship is preserved either way). Constraints from the maintainers, which are firm:

1. One run-scoped engine-health mechanism, N detectors. API-terminal errors (402/balance/auth — this PR) and spawn-level failures (Spawn failures are infrastructure evidence, not model evidence #35/Circuit-break an engine after its first spawn failure #36) are two detectors feeding one breaker, not two breakers.
2. Log everything, credit nothing. Skipped/infra attempts write visible eval rows with distinct verdicts (ENGINE_DOWN, INFRA) that scoreboard aggregation excludes from model stats — same doctrine as the unattributed-rows quarantine. No silent suppression of rows.
3. The display contract must learn the new states. task_state_bucket(), the run summary, final artifact pages, and Ringside must render engine-down/infra runs truthfully — a dead-engine run that displays as "passed" or "waiting" is a wrong display, which is the one bug class this project treats as unforgivable.
4. False positives are worse than false negatives. A worker whose output text legitimately discusses billing errors must not kill its engine. Narrow the matchers (and cover the plain-text HTTP 402 case the current regex misses); prefer structured signals where they exist.
5. Per-PR feedback still applies: single-probe admission so concurrent queued tasks can't all race past the breaker check (Circuit-break an engine after its first spawn failure #36), and either implement engine-aware cancellation of in-flight tasks or claim only "no retries" (Fail fast on terminal engine-down errors (402/balance exhausted/auth-expired) #33).

Fail-fast-on-dead-engine is a real waste-killer and we'd like it in main soon — happy to review a joint design sketch in either thread before you build. Thanks to you both for attacking the same real problem from opposite ends.

@mlava — sorry for the slow reply, and thanks for the sketch. I'm in, and I think your shape is right: one run-scoped breaker, two detectors, visible-but-unranked rows, single-probe admission. I also agree with your packaging preference — one co-authored PR superseding #33/#35/#36. #35's row-suppression rule dying in favor of visible rows means the stack gets rebuilt from the bottom regardless, so layering would be ceremony.

You asked me to correct anything wrong about #33's internals. Four things, and one of them changes the design rather than just the description.

1. There are no structured signals in #33 today — Detector B is content-only. Your sketch says "structured signals where they exist, with Jeff's tail-scoped text patterns as fallback." Worth being explicit that the fallback is currently the whole detector. What exists is a regex table scanned against the last 2000 chars of combined stdout/stderr, gated on not timed_out and returncode != 0. No engine emits anything structured today that we could prefer over text.

2. The 402 gap is wider than "plain-text HTTP 402." I probed the live matcher; only quoted-JSON "status": 402 / "code": 402 and the literal words "payment required" hit:

'HTTP/1.1 402 Payment Required'                 -> payment_required   (matches on the words, not the code)
'{"error": {"status": 402}}'                    -> http_402
'HTTP 402'                                      -> None
'request failed with status 402'                -> None
'Error: 402'                                    -> None
'opencode: error: exit status 402'              -> None

Any engine that surfaces the bare status code without the reason phrase is invisible to it.

3. The important one: widening the 402 matchers makes the false-positive problem strictly worse, so the fallback needs the same corroboration treatment you designed for Detector A. The narration case is not hypothetical:

'I implemented the 402 Payment Required handler and it works'  -> payment_required

That's constraint 4 firing on the current matcher. Two things keep it from being live today: the scan is tail-scoped, and the call site requires a nonzero exit — a worker that narrates about billing and then succeeds can't trip anything. So the exposure is narrower than the raw matcher suggests, but it's real for any task that discusses payment/auth handling and then fails its check for unrelated reasons. And it gets worse the moment we widen 402 to catch case 2, because a bare-number matcher will hit a diff, a log excerpt, or a test fixture.

So I don't think "narrow the matchers" alone can satisfy both 2 and 4 — the table has to get broader to fix the gap and the decision has to get narrower to fix the false positive. The corroboration signal is already sitting there unused: parse_token_count runs one line above the detector call and its result is discarded for this purpose. Token evidence means a model actually answered, which is close to proof the engine is not down on billing/auth. Proposed rule for Detector B, symmetric with your Detector A hardening:

nonzero exit and no token usage and a pattern hit in the tail → ENGINE_DOWN; a pattern hit with token evidence present is narration, not a dead engine.

That buys us room to widen the patterns to bare status codes without the diff-and-fixture false positives.

4. My PR body overstates in-flight behavior — you're right to claim only "no new spawns, no retries." #33 checks the breaker in exactly two places: before taskdir prep, and at the top of each attempt. A task already inside _run_worker when the breaker trips runs to completion and is then classified on exit. It never cancels anything. The body says "already running or still queued," which is wrong for the running case; that's the maintainers' point 5 and I'll take the honest claim.

Related, and worth flagging against your admission gate: #33's end-to-end test runs at max_parallel: 1 for determinism, so the wave race is the one thing it has never exercised. At any real parallelism, N tasks clear the pre-prep check before the first verdict lands. Your single-probe gate is the fix, and the joint PR should have a test that actually runs at max_parallel > 1 and asserts the queued tasks held.

5. Constraint 3 is already failing in #33 as written, and I'd missed it. The maintainers' example of the unforgivable display bug — "a dead-engine run that displays as 'passed' or 'waiting'" — is not hypothetical, it's what my PR does right now:

task_state_bucket('engine-down')  -> 'waiting'      task_state_word -> 'waiting'
task_state_bucket('infra')        -> 'waiting'      task_state_word -> 'waiting'

Both new statuses fall through the if chain to the default return "waiting", and six call sites downstream inherit it — the progress bar, the task rows, the pass/fail counts. So a run where the engine died on billing currently renders as a run that's still waiting to start. Same for INFRA when #36's statuses land. This needs a bucket of its own rather than a new branch on one of the existing four, since these tasks are neither working nor failed in any sense the display should conflate.

Proposed split, if it suits you:

• You: the breaker core and admission gate — per-engine state under the runner lock, single-probe hold/release, and the detector interface they call into. Plus Detector A as written, with your corroboration hardening. It's your skeleton and your design; I'd rather extend it than re-derive it.
• Me: everything downstream of the breaker. Detector B (widened patterns + the token-corroboration rule above) behind your interface; the aggregation exclusions, which Fail fast on terminal engine-down errors (402/balance exhausted/auth-expired) #33 already has working for ENGINE_DOWN and generalizes to INFRA for free; and the whole display contract — task_state_bucket(), the run summary and its actionable hint, final artifact pages, Ringside. Constraint 3 is the one the maintainers called unforgivable to get wrong, so I'd rather own it outright than leave it to whoever reaches the end of the diff.
• Me, also: the parallelism test. Fail fast on terminal engine-down errors (402/balance exhausted/auth-expired) #33's coverage is max_parallel: 1, which is exactly the case your admission gate doesn't need to exist for. I'll write the one that runs at real parallelism and asserts queued same-engine tasks actually held behind the probe — for both detectors, since a wave of spawn failures races identically.

Whichever of us opens the PR, co-author trailers for both and it supersedes all three of these.

Maintainers — if this shape and split look right, we'll build against it. The one open question I'd want your call on before we start: does the token-corroboration rule in point 3 satisfy constraint 4 for you, given it lets the pattern table get broader than it is today?